Update to Swagger 1.2. Fixes #11, #8, #7. by STRML · Pull Request #14 · strongloop/loopback-component-explorer

STRML · 2014-07-04T22:13:27Z

Changes in this PR:

Moved public (swagger UI) out of this module and required swagger-ui as a module in package.json.
Added swaggerDistRoot option so users of the API Explorer can override any part of the UI (styles, JS, HTML).
Compatibility with Swagger 1.2 (spec) (transition guide)
Model schemas are now correctly populated.
An accessToken can be provided via the header input.

When this is merged, it should be safe to remove strong-remoting/ext/swagger, the associated test, and app.docs().

Fixes #7, #8, #11.

Added option `swaggerDistRoot` for specific file overrides.

…module. Will now correctly return model schemas. Moved swagger.js tests to this module.

slnode · 2014-07-04T22:13:29Z

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

raymondfeng · 2014-07-05T07:38:19Z

@STRML, thank you for the patch. This is a great improvement to our API explorer experience. I started to play with it and a few issues are fixed at:

https://github.com/strongloop/loopback-explorer/tree/STRML-feature/swagger1.2

The PR also exposes some requirements to strong-remoting. For example, the accepts/returns argument should have a way to reference the declaring model as the type. Let's try to create issues as we go.

- Added array items definition. - Ignored body when specified via arg.http.source.

STRML · 2014-07-05T16:15:09Z

Ok. I've done a little bit of cleanup for array items. I'll create corresponding issues in strong-remoting and link them back here.

Added comments, api version, and better Swagger 1.2 compat.

Route.accepts & route.returns can now share these translations.

bajtos · 2014-07-09T16:19:46Z

@slnode add to whitelist

raymondfeng · 2014-07-09T16:21:36Z

@STRML We are losing the CSS customization made by loopback-explorer. Is there a way to add it back?

STRML · 2014-07-09T16:23:18Z

@raymondfeng Yes, any loopback-explorer overrides of swagger-ui go in /public. I will find the old customizations & restore them.

TODO extract overrides instead of replacing styles wholesale.

STRML · 2014-07-09T17:05:11Z

@raymondfeng I've replaced the old styles.css and supporting files in 45172da. At some point someone should go through it and move the individual overrides out.

bajtos · 2014-07-09T18:08:15Z

I did a quick read through the patch, I am impressed, especially about the comments. I'd like to do a more detailed review tomorrow.

There is one idea we had with @ritch connected to the migration of swagger code from strong-remoting to loopback-explorer: change the way how resource descriptors are exposed - use regular express routes instead of building a virtual shared object & shared methods. Reasoning: at the moment, loopback-explorer is modifying the application by adding more remotable objects. This also means that to access swagger descriptors, one has to pass the authentication layer configured for regular models. We had to add a patch to loopback to fix this (see lib/application.js).

@STRML what's your opinion on this idea? Do you agree? Does it make sense to make the change as part of this pull request, or would you rather leave it for later?

raymondfeng · 2014-07-09T18:33:42Z

@STRML Fantastic! I was able to bring up the new explorer with existing application. The changes are merged into https://github.com/strongloop/loopback-explorer/tree/STRML-feature/swagger1.2. Would you like to rebase from it to avoid future merging conflicts?

STRML · 2014-07-09T19:36:34Z

@bajtos I agree. That should be easy to fit into this patch. I'll update shortly.
@raymondfeng Thanks - will rebase as your commit is good context as to why body is not ignored. I need to reformat the commit messages, however, and will update this PR.

STRML · 2014-07-09T22:39:42Z

@bajtos I've completed that change. It significantly reduces the complexity of the implementation. Review when ready. 👍

This completes the migration of swagger processing from strong-remoting into loopback-explorer. Added additional usage instructions to README and additional testing. This commit introduces a change into where resource descriptors are hosted. They are no longer hosted under /swagger, but instead under the same path as the Explorer, wherever that may be. Generally, the resource listing will be available at /explorer/resources, and api listings under /explorer/resources/{modelName}.

bajtos · 2014-07-10T07:00:28Z

This is not entirely true. Types can be specified both by constructors (String) and type names (object).

- Supports all three LDL array type specifications. - Added model-helper tests. - translate-keys no longer modifies the incoming object. - Non-primitive types are now correctly converted into a primitive with a `format` attribute. - `id` properties are not assumed to be required if they are generated.

- Uses model-helper to parse types for swagger. - Separated returns & accepts hacks. - Documentation fixes - TODO add param regex

Not necessary since this can easily be done with an app.use() before calling loopback-explorer.

bajtos · 2014-07-11T10:01:44Z

I think you misunderstood my comment. The idea was to not modify the route, but return the accepts in the format needed by swagger.

var accepts = routeHelpers.convertAcceptsToSwagger(route, classDef); var apiDoc = { // ... parameters: accepts.map(routeHelper.acceptToParameter(route)) // ... }

An even better solution may be to call acceptToParameter from hackAcceptsDefinition.

function buildParameters(route) { // add ctor arguments // filter out computed arguments // for each argument: convert the type to Swagger, convert the argument to Swagger parameter } // later var apiDoc = { // ... parameters: buildParameters(route) // ... }

Anyways, I guess this is rather subjective. If you don't think my proposal will make the code better or you don't think it is worth the effort, I can live with the current version too.

bajtos · 2014-07-11T10:13:54Z

I left few more comments, the patch is almost good to be merged. I'd like to test it a little bit before giving it a green light.

STRML · 2014-07-11T14:05:14Z

@bajtos Addressed your comments; should be good to go.

raymondfeng · 2014-07-11T15:12:30Z

loopback-explorer works with loopback 1.x which depends on express 3.x. Can we make it compatible with loopback 1.x & 2.x?

AFAIK express 4.x apps have the same public api as the 3.x apps, i.e. function(req, res), thus this change was not strictly necessary.

STRML · 2014-07-11T18:22:41Z

@raymondfeng Thanks for the comments, fixed.

bajtos · 2014-07-14T20:01:02Z

The code LGTM. Unfortunately I don't have time to test the new explorer myself, I'll leave the final sign-off to Raymond.

Thank you @STRML for your contribution, I appreciate the effort you put into this patch and how quickly you addressed our comments.

raymondfeng · 2014-07-14T20:23:12Z

The authorization key name should be access_token.

raymondfeng · 2014-07-14T20:38:04Z

@STRML please fix the last issue I raised above. We're good to go after that. Thanks!

STRML · 2014-07-14T20:40:41Z

Good to go, merge when ready.

Update to Swagger 1.2. Fixes #11, #8, #7.

STRML · 2014-07-15T00:53:24Z

Thanks, everyone, for helping me get this merged and available to all of loopback's users!

Samuel Reed added 2 commits July 4, 2014 14:28

Load swagger ui from swagger-ui package instead.

56003f0

Added option `swaggerDistRoot` for specific file overrides.

Swagger 1.2 compatability. Moved strong-remoting/ext/swagger to this …

eb31787

…module. Will now correctly return model schemas. Moved swagger.js tests to this module.

Fix api resource path and type ref to models.

2f8506c

Some swagger 1.2 migration cleanup.

67bb10b

- Added array items definition. - Ignored body when specified via arg.http.source.

STRML mentioned this pull request Jul 5, 2014

Additions to accept/returns type definitions strongloop/strong-remoting#79

Closed

Raymond Feng and others added 3 commits July 5, 2014 10:21

Make sure body parameter is shown.

d34304a

Refactoring swagger 1.2 rework.

4c0ce42

Added comments, api version, and better Swagger 1.2 compat.

Refactor key translations between LDL & Swagger.

a4a36f5

Route.accepts & route.returns can now share these translations.

STRML added 3 commits July 9, 2014 11:45

Allow easy setting of accessToken in explorer UI.

5f22982

Restore existing styles.

a6afe13

TODO extract overrides instead of replacing styles wholesale.

Fix missing strong-remoting devDependency.

19c3fe3

bajtos reviewed Jul 10, 2014
View reviewed changes

STRML added 7 commits July 10, 2014 12:07

Refactor route-helper & add tests.

3ce35e1

- Uses model-helper to parse types for swagger. - Separated returns & accepts hacks. - Documentation fixes - TODO add param regex

Rename translateKeys to translateDataTypeKeys.

781ac2b

Add url-join so path.join() doesn't break windows

75713f1

Remove peerDependencies, use express directly.

cbf768f

Remove swagger.test.js license

34b3983

Remove preMiddleware.

6224243

Not necessary since this can easily be done with an app.use() before calling loopback-explorer.

bajtos reviewed Jul 11, 2014
View reviewed changes

STRML added 3 commits July 11, 2014 09:55

More consise type tests

75ba058

Simplify accepts and returns hacks.

4b3785c

Remove forgotten TODO.

a857fe5

raymondfeng reviewed Jul 11, 2014
View reviewed changes

Fix debug namespace, express version.

5c130a4

Fix resources if the explorer is at a deep path.

cefbdb3

raymondfeng reviewed Jul 14, 2014
View reviewed changes

Comment thread public/lib/loadSwaggerUI.js Outdated

raymondfeng Jul 14, 2014

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The authorization key name should be access_token.

s/accessToken/access_token in authorization key name

fcd1926

raymondfeng added a commit that referenced this pull request Jul 14, 2014

Merge pull request #14 from STRML/feature/swagger1.2

cd873e9

Update to Swagger 1.2. Fixes #11, #8, #7.

raymondfeng merged commit cd873e9 into strongloop:master Jul 14, 2014

bajtos mentioned this pull request Jul 22, 2014

Remove ext/swagger strongloop/strong-remoting#83

Merged

STRML mentioned this pull request Aug 12, 2014

Model Schemas #8

Closed

Uh oh!

Conversation

STRML commented Jul 4, 2014

Uh oh!

slnode commented Jul 4, 2014

Uh oh!

raymondfeng commented Jul 5, 2014

Uh oh!

STRML commented Jul 5, 2014

Uh oh!

bajtos commented Jul 9, 2014

Uh oh!

raymondfeng commented Jul 9, 2014

Uh oh!

STRML commented Jul 9, 2014

Uh oh!

STRML commented Jul 9, 2014

Uh oh!

bajtos commented Jul 9, 2014

Uh oh!

raymondfeng commented Jul 9, 2014

Uh oh!

STRML commented Jul 9, 2014

Uh oh!

STRML commented Jul 9, 2014

Uh oh!

bajtos Jul 10, 2014

Choose a reason for hiding this comment

Uh oh!

bajtos Jul 11, 2014

Choose a reason for hiding this comment

Uh oh!

bajtos commented Jul 11, 2014

Uh oh!

STRML commented Jul 11, 2014

Uh oh!

raymondfeng Jul 11, 2014

Choose a reason for hiding this comment

Uh oh!

bajtos Jul 14, 2014

Choose a reason for hiding this comment

Uh oh!

STRML commented Jul 11, 2014

Uh oh!

bajtos commented Jul 14, 2014

Uh oh!

raymondfeng Jul 14, 2014

Choose a reason for hiding this comment

Uh oh!

raymondfeng commented Jul 14, 2014

Uh oh!

STRML commented Jul 14, 2014

Uh oh!

STRML commented Jul 15, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants